Skip to content

fix(credentials): clear stored refs on credential delete to prevent silent cascade orphaning#4418

Merged
waleedlatif1 merged 3 commits intostagingfrom
waleedlatif1/slack-cred-deselect
May 3, 2026
Merged

fix(credentials): clear stored refs on credential delete to prevent silent cascade orphaning#4418
waleedlatif1 merged 3 commits intostagingfrom
waleedlatif1/slack-cred-deselect

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Centralize credential deletion in lib/credentials/deletion.ts — clears stored refs across workflow_blocks, deployment versions, paused executions, checkpoints, and knowledge connectors before deleting the row
  • Wire helper into OAuth disconnect, explicit credential DELETE, and copilot delete paths so FK CASCADE never silently orphans references
  • Add CREDENTIAL_CREATED / CREDENTIAL_RECONNECTED audit actions for full credential lifecycle traceability
  • Extend CREDENTIAL_SUBBLOCK_IDS to include manualCredential (latent migration gap)

Type of Change

  • Bug fix

Testing

Tested manually. Added unit tests for the recursive walker covering subBlock, tool params, deployment-style, and checkpoint shapes.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 3, 2026 3:31am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 3, 2026

PR Summary

Medium Risk
Touches credential deletion paths and performs cross-table JSON updates to clear references before deletes; mistakes could lead to unintended data mutations or missed cleanup, but scope is limited to credential lifecycle flows.

Overview
Centralizes credential deletion into lib/credentials/deletion.ts, which clears stored credential references across workflow editor state (workflow_blocks), deployment versions, paused executions, checkpoints, and knowledge connectors before deleting the credential row and recording a deletion audit entry.

Updates the OAuth disconnect route and the credential DELETE API (plus the Copilot manage-credential delete tool) to use deleteCredential, ensuring credentials are removed before deleting related account rows to avoid FK cascade leaving stale JSON references.

Adds audit coverage for the OAuth credential lifecycle by recording CREDENTIAL_CREATED on draft-based creation and introducing a new CREDENTIAL_RECONNECTED action during reconnects; also expands CREDENTIAL_SUBBLOCK_IDS to include manualCredential and adds unit tests for the recursive reference-clearing walker.

Reviewed by Cursor Bugbot for commit cb1efeb. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 3, 2026

Greptile Summary

This PR centralizes credential deletion into a new lib/credentials/deletion.ts helper that clears stored refs across workflow blocks, deployment versions, paused executions, checkpoints, and knowledge connectors before removing the credential row — preventing silent FK-cascade orphaning. It also wires in CREDENTIAL_CREATED / CREDENTIAL_RECONNECTED audit actions and extends CREDENTIAL_SUBBLOCK_IDS to cover manualCredential. The approach is idempotent and consistent across all three deletion call-sites (OAuth disconnect, explicit DELETE route, copilot tool).

Confidence Score: 5/5

Safe to merge — all findings are P2 (performance/observability) with no correctness or security defects.

No P0 or P1 issues found. The deletion logic is idempotent, authorization is verified at every call site before deleteCredential is invoked, and the recursive walker is thoroughly unit-tested. The two P2 comments are minor performance and audit-completeness suggestions.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/credentials/deletion.ts New central deletion helper — fetches credential row, clears refs across 5 tables in parallel, deletes row, and fires audit. Logic is sound and idempotent; walker correctly scopes by workspaceId for all table scans.
apps/sim/app/api/auth/oauth/disconnect/route.ts Reworked to explicitly delete credential refs before account rows. Correctness is good; outer credential loop is sequential (P2 performance).
apps/sim/app/api/credentials/[id]/route.ts Replaces bare db.delete with deleteCredential helper; audit record is now generated inside the helper. The env-credential path retains its own audit — consistent and correct.
apps/sim/lib/copilot/tools/handlers/management/manage-credential.ts Wires deleteCredential into the copilot delete path with proper auth checks, but omits actorName/actorEmail (P2).
apps/sim/lib/credentials/draft-hooks.ts Adds CREDENTIAL_CREATED and CREDENTIAL_RECONNECTED audit records; userId param added to handleReconnectCredential to support the new audit entry — clean fix.
apps/sim/lib/credentials/tests/deletion.test.ts Comprehensive unit tests covering all walker shapes (subBlock, tool params, deployment, checkpoint, multi-reference). Good coverage of edge cases including same-reference return and primitives.
apps/sim/lib/workflows/persistence/utils.ts Exports CREDENTIAL_SUBBLOCK_IDS and adds manualCredential to close the migration gap; straightforward and correct.
packages/audit/src/types.ts Adds CREDENTIAL_RECONNECTED audit action in the correct position within the credential action group.
packages/testing/src/mocks/audit.mock.ts Mock updated to match the new CREDENTIAL_RECONNECTED action; reordering of CREDENTIAL_DELETED has no functional impact.
apps/sim/lib/credentials/draft-processor.ts One-line change to thread userId through to handleReconnectCredential — correct and minimal.

Sequence Diagram

sequenceDiagram
    participant C as Caller (disconnect / DELETE / copilot)
    participant D as deleteCredential()
    participant DB as Database

    C->>+D: deleteCredential({ credentialId, actorId, reason })
    D->>DB: SELECT credential WHERE id = credentialId
    DB-->>D: row (workspaceId, type, displayName, ...)
    D->>+DB: Promise.all clearCredentialRefs(credentialId, workspaceId)
    Note over DB: 5 parallel passes
    DB-->>-D: workflow_blocks, deployment_versions, paused_executions, checkpoints, knowledge_connectors updated
    D->>DB: DELETE credential WHERE id = credentialId
    DB-->>D: ok
    D--)C: recordAudit(CREDENTIAL_DELETED) [fire-and-forget]
    D-->>-C: void
Loading

Reviews (2): Last reviewed commit: "improvement(credentials): parallelize in..." | Re-trigger Greptile

Comment thread apps/sim/lib/credentials/deletion.ts
Comment thread apps/sim/lib/credentials/deletion.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cb1efeb. Configure here.

@waleedlatif1 waleedlatif1 merged commit 68f66ba into staging May 3, 2026
14 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/slack-cred-deselect branch May 3, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant